Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for serializing json api resource collections into root key #23

Merged
merged 1 commit into from
Feb 24, 2014

Conversation

kjg
Copy link
Contributor

@kjg kjg commented Feb 19, 2014

How do you feel about this schema for json api resource collections?

schema do
  type 'users'
  entities :users, item, USER_SERIALIZER, {:type => :resource_collection}
 end

@kjg
Copy link
Contributor Author

kjg commented Feb 19, 2014

Hold off on merging this. I'm formulating some additional thoughts.

@kjg
Copy link
Contributor Author

kjg commented Feb 20, 2014

I'm not thrilled with using the context hash this way, but I can't come up with anything better. Have any ideas? If you like it as is feel free to merge.

@kjg
Copy link
Contributor Author

kjg commented Feb 20, 2014

The only other thing I can think of is writing a new method on the json_api adapter called "Collection" instead of using entities for this. I'm not sure if that is any better.

@ismasan
Copy link
Owner

ismasan commented Feb 21, 2014

I think this brings us back to this discussion. See my proposal there re. a new collections method in the adapters DSL.

I would try to avoid overloading the behaviour of the DSL by providing hash options and instead try to come up with a DSL that expresses the most common use cases across media types. One useful distinction can be the following:

  • DSL methods express the structure of a generic message-oriented payload (type, properties, collections, entities, etc).
  • The context hash includes options to do with the concrete data in a given payload (controller, request data, conditional modifiers, current user, etc etc)

So going back to my previous proposal I'd opt for extending the DSL with

collection :users, item, UserSerializer
entities :accounts, item.accounts, AccountSerializer

The default implementation of collection would just be an alias to entities, whereas in JsonAPI:

  • collection adds objects as the main collection property of the payload. Note that this could set the type implicitely, because in JsonAPI the key for the collection is the pluralized version of the main resource name ("users").
  • entities would always add objects to the linked property of the payload.

Also note that, in JsonAPI, items in the main collection can have references to objects in the linked lookup object. This means that the adapter should be able to add entities while building the main collection array. Something like (not necessarily the actual code):

collection :users, item do |user, user_serializer|
  user_serializer.name user.name
  # this adds the account entity to the top "linked" object
  user_serializer.reference :account, user.account, AccountSerializer
  # this embeds the entity into the current object, as normal
  user_serializer.entity :company, user.company, CompanySerializer
end

The reference (or some other name) method could be an alias to entity in the default implementation.

@kjg
Copy link
Contributor Author

kjg commented Feb 21, 2014

Sounds great. I'll work on these changes.
On Feb 21, 2014 7:42 AM, "Ismael Celis" notifications@github.com wrote:

I think this brings us back to this discussionhttps://github.com//issues/9#issuecomment-33682990.
See my proposal there re. a new collections method in the adapters DSL.

I would try to avoid overloading the behaviour of the DSL by providing
hash options and instead try to come up with a DSL that expresses the most
common use cases across media types. One useful distinction can be the
following:

  • DSL methods express the structure of a generic message-oriented
    payload (type, properties, collections, entities, etc).
  • The context hash includes options to do with the concrete data in a
    given payload (controller, request data, conditional modifiers, current
    user, etc etc)

So going back to my previous proposal I'd opt for extending the DSL with

collection :users, item, UserSerializerentities :accounts, item.accounts, AccountSerializer

The default implementation of collection would just be an alias to
entities, whereas in JsonAPI:

  • collection adds objects as the main collection property of the
    payload. Note that this could set the type implicitely, because in
    JsonAPI the key for the collection is the pluralized version of the main
    resource name ("users").
  • entities would always add objects to the linked property of the
    payload.

Also note that, in JsonAPI, items in the main collection can have
references to objects in the linked lookup object. This means that the
adapter should be able to add entities while building the main
collection array. Something like (not necessarily the actual code):

collection :users, item do |user, user_serializer|
user_serializer.name user.name

this adds the account entity to the top "linked" object

user_serializer.reference :account, user.account, AccountSerializer

this embeds the entity into the current object, as normal

user_serializer.entity :company, user.company, CompanySerializerend

The reference (or some other name) method could be an alias to entity in
the default implementation.

Reply to this email directly or view it on GitHubhttps://github.com//pull/23#issuecomment-35730913
.

@kjg
Copy link
Contributor Author

kjg commented Feb 21, 2014

Okay. I've made the changes using collection. I don't think it is ever valid to have an entity embedded in the current object instead of being added to the [:linked] objects, so calling entity or entities within the collection will always add them to :linked.

I can clean all this up significantly once serializer_from_block_or_class returns a serializer object instead of a hash, specifically so I have access to the root_name of the entities.

@@ -55,12 +55,27 @@ def entities(name, collection, serializer_class = nil, context_options = {}, &bl
end
end

def collection(name, collection, serializer_class = nil, context_options = {}, &block)
@treat_as_resource_collection = true
data[:resource_collection] = [] unless data[:resource_collection].is_a?(Array)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just

data[:resource_collection] ||= []

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to
Adapter#initalize

#adapter.rb
  def initialize(serializer)
    @serializer = serializer
    @data = Hash.new{|h,k| h[k] = {}}
  end

data[:resource_collection] will be a hash the first time it is accessed.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. And that's my own code :)

@kjg
Copy link
Contributor Author

kjg commented Feb 24, 2014

I'd love to uses these changes in the project I'm working on. Are there any further changes you'd like me to make, or is this good to go now?

@ismasan
Copy link
Owner

ismasan commented Feb 24, 2014

Good to go as it makes the JsonAPI functional. Will merge and release in a minute. Are we still extracting JsonAPI into a separate gem at some point or do you prefer to wait until the serializer_from_block_or_class issue is sorted?

ismasan pushed a commit that referenced this pull request Feb 24, 2014
Allow for serializing json api resource collections into root key
@ismasan ismasan merged commit 59514ed into ismasan:master Feb 24, 2014
@kjg
Copy link
Contributor Author

kjg commented Feb 24, 2014

I prefer to wait until serializer_from_block_or_class is sorted, since it is a significant api change.

@kjg kjg deleted the json_collections branch March 25, 2014 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants